refactor(settings): unify sidebar, data grid, and filter settings storage and UI#1847
Conversation
…older stores with it
…pace the selected-pane key
…er-table layouts and filters
…rState as the single connection-scoped state
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3b5326894
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cache[key.connectionId] = entries | ||
| writeEntries(entries, for: key.connectionId) | ||
| } | ||
| syncTracker.markDeleted(.settings, id: Self.syncCategory(for: key.storageKey)) |
There was a problem hiding this comment.
When a user resets a synced table layout or clears a hidden-only customization, this records a .settings tombstone, but performPush only serializes dirty settings records and never turns settings tombstones into recordIDsToDelete; applyRemoteChanges also ignores deleted Settings_ IDs. In that scenario the old CloudKit layout record is not removed, so another device or a later pull can re-apply the column widths/order/hidden columns the user just reset.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| static func syncCategory(for storageKey: String) -> String { | ||
| syncCategoryPrefix + storageKey |
There was a problem hiding this comment.
For tables with long database/schema/table identifiers, this embeds the full percent-encoded storageKey in the settings category, which SyncRecordMapper.recordID later uses as Settings_<category>. The CKRecord.ID docs document a 255-character ASCII limit for record names; a 63-character database, schema, and table name already exceeds that after the Settings_columnLayout. prefix and encoded UUID, so CloudKit can reject layout sync for those tables. Use a bounded stable ID, such as a hash, and keep the full scope in the payload.
Useful? React with 👍 / 👎.
# Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69dd1f1be1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .frame(minWidth: 180, idealWidth: 210, maxWidth: 250) | ||
|
|
||
| ThemeEditorView(selectedThemeId: effectiveThemeIdBinding) | ||
| ThemeEditorView(selectedThemeId: slotThemeBinding) |
There was a problem hiding this comment.
When the current effective appearance is light and the user switches the new Editing picker to Dark (or vice versa), this passes the inactive slot's binding into ThemeEditorView, but that view still renders and edits ThemeEngine.shared.activeTheme rather than selectedThemeId. The result is that the left list changes the dark preference while the right editor continues to show/edit the active light theme, so users cannot reliably customize the non-active slot from this pane.
Useful? React with 👍 / 👎.
| guard let scope = TableScope(storageComponent: storageKey), | ||
| let entry = try? decoder.decode(PersistedColumnLayout.self, from: data) else { return } | ||
| var entries = loadEntries(for: scope.connectionId) | ||
| entries[storageKey] = entry |
There was a problem hiding this comment.
On upgraded devices that still have com.TablePro.columns.hiddenColumns.* in UserDefaults, applying a synced layout whose hiddenColumns is absent (the format produced after hidden columns are cleared while widths/order remain) leaves that legacy key intact. The next loadHiddenColumns sees nil in the new entry and falls through to migrateLegacyHidden, re-hiding stale columns and marking them dirty again, so a remote clear of column visibility does not stick for those tables.
Useful? React with 👍 / 👎.
Revert the Settings window to the default tab bar and fold the new panes in: Sidebar options move to General, and the data grid, history, and Saved Customizations share one Data tab.
Keep the Data tab doc moves while adopting main's updated row cap wording: the query is sent as written and TablePro stops reading at the cap (#1884).
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f68888ef78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return files | ||
| .filter { $0.pathExtension == "json" } | ||
| .map { $0.deletingPathExtension().lastPathComponent } | ||
| .filter { !$0.hasSuffix(".browse") } |
There was a problem hiding this comment.
When the saved-filter table name is literally browse, its normal filter file is named <scope>.browse.json, so after deleting the .json extension the scope key also ends with .browse. This filter removes that real table from SavedCustomizationsService.all(), leaving users unable to see or reset saved filters for such tables in Settings > Data; the browse-search sidecar needs to be distinguished without a raw suffix match.
Useful? React with 👍 / 👎.
| schemaName: scope.schema, | ||
| tableName: scope.table | ||
| )) | ||
| filterStore.clearLastFilters( |
There was a problem hiding this comment.
In the Settings > Data reset flow, SavedCustomizationsSection calls reset() and immediately reloads the list, but clearLastFilters removes the JSON file on FilterSettingsStorage's async ioQueue while customizedStorageKeys() reads the directory synchronously. For filter-only rows, Reset/Reset All can therefore reload before the deletion runs and show the supposedly reset customization until a later refresh; this path should wait for pending disk writes or make the reset deletion synchronous before reloading.
Useful? React with 👍 / 👎.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
1 participant
What and why
Settings for the sidebar, data grid, and filters were scattered across UserDefaults keys, JSON files, and ad-hoc storage, with no shared scoping and no single place to review or reset them. This branch unifies the storage foundation, reorganizes the Settings window, and fixes several bugs that fell out of the old layout. Native only (AppKit/SwiftUI, HIG), no compatibility shims, no PluginKit ABI changes. Old keys are migrated (read-old / write-new), so no saved user settings are lost.
User-facing changes
Added
Fixed (root-cause)
Internal refactors
DefaultsKey/PreferenceKeys/KeyValueStore), backing linked-folder and other stores.TableScopecomposite key with reversible percent-encoded storage components;CompositeStorageKeydelegates to it.ColumnLayoutPersisting), replacing the split visibility persister.SyncScopegate onSyncChangeTracker.markDirty; additive per-table cases inSyncCoordinator.ConnectionStorage/AIKeyStorage/LicenseStorage/SSHProfileStorage.SQLBoundaryValidator.SidebarViewModelsearch text and Redis tree forwarded into the connection-scopedSharedSidebarState(single source of truth).accentColortheme field and a duplicate accessibility observer.Testing
TableScopedecode, column-layout sync round-trip, filter match-mode persistence, saved-customizations service).--strict).main(KeychainHelper overwrite, DataChangeManager reload version, 2 TableQueryBuilderFilteredQuery). They are not regressions from this work.Verify before release
These surfaces are correct by unit test and build, but should get a pass in a running app before shipping: